Refactor SourceType handling to enum by Trenly · Pull Request #6313 · microsoft/winget-cli

Trenly · 2026-06-21T21:09:37Z

📖 Description

This refactor moves internal source-type handling to Repository::SourceType enum paths across repository, CLI, and COM layers, while preserving string conversion only at external boundaries (CLI/JSON/GP/COM input-output). It also removes accidental string-based source construction paths and updates source-related tests/fixtures to use canonical source type values.

Key changes include:

SourceDetails.Type and internal source factory routing now use SourceType.
ISourceFactory::GetForType now takes SourceType.
Source constructor string-type overload removal and default-type string helper removal at call sites.
Source parsing/serialization boundaries explicitly convert using TryConvertToSourceTypeEnum/SourceTypeEnumToString.
Source-related tests updated for enum semantics, including legacy tombstone/override source-list parsing compatibility.

Copilot assisted with implementation.

🔗 References

Resolves #4463

🔍 Validation

Executed source-focused test subset:
- ./src/x64/Debug/AppInstallerCLITests/AppInstallerCLITests.exe -v high -d yes 'Source'

✅ Checklist

Signed the Contributor License Agreement
Linked to an issue
Updated Release Notes (if applicable)
Updated documentation (if applicable)
Updated Copilot instructions (if build, architecture, or conventions changed)

📋 Issue Type

Bug fix
Feature
Task

Microsoft Reviewers: Open in CodeFlow

Migrate SourceDetails and source factory internals to use SourceType enum paths while keeping string conversion at input/output boundaries. Align CLI/COM/repository flows and source-related tests with canonical enum conversion behavior, and keep legacy user source tombstone/override parsing behavior intact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

florelis

I haven't finished reviewing. My main comment so far is: Why move where in the code empty types are handled? We could have kept it the same, and then you wouldn't have as many logic changes to verify.

florelis · 2026-06-29T20:23:07Z


 ## Bug Fixes

+* Refactored source type handling to use a `SourceType` enum with centralized string conversion, and added stronger `source add` type validation (#4463).


I'm not sure we need to mention this here. An internal refactor doesn't seem too relevant to someone asking "what's new in this release?"

Fair - 🤖 was just following default instructions, will remove

florelis · 2026-06-29T20:23:51Z

+        PredefinedInstalled,
+        PredefinedWriteable,
+        PackageTracking,
+        ConfigurableTest,


Should this be gated by the test hooks being enabled?

I opted to leave the enum alone and have the test hooks handle the implementation / backend so that enum indexes don't change between debug / test hook builds and release builds

florelis · 2026-06-29T21:04:29Z

+    std::optional<SourceType> TryConvertToSourceTypeEnum(std::string_view sourceType);
+
+    // Converts a SourceType enum to the corresponding canonical string.
+    std::string_view SourceTypeEnumToString(SourceType sourceType);


Nit: Is this the naming scheme we usually follow for these enum<->string conversions? I see the one below is just ToString

florelis · 2026-06-29T21:05:13Z

+        // Defaults to PreIndexedPackage if not specified, moved here from call sites during migration from
+        // String type to Enum type to avoid conversion in multiple places.


Is there any reason we'd need this historical note?

Probably not - but I thought it might help a review

florelis · 2026-06-29T21:05:49Z

+    // Converts a source type string to the corresponding SourceType enum.
+    SourceType ConvertToSourceTypeEnum(std::string_view sourceType);
+
+    // Attempts to convert a source type string to the corresponding SourceType enum.
+    std::optional<SourceType> TryConvertToSourceTypeEnum(std::string_view sourceType);


Nit: Do we need both? Do we usually have both?

We usually don't have both, and instead have <Enum>::Unknown as the fallback. The reason for having both here was to avoid needing the Unknown type in the enum

florelis · 2026-06-29T23:49:45Z

+    std::unique_ptr<ISourceFactory> ISourceFactory::GetForType(SourceType type)
    {
 #ifndef AICLI_DISABLE_TEST_HOOKS
        // Tests can ensure case matching


Remove outdated comment

florelis · 2026-06-29T23:50:03Z

            return Microsoft::ConfigurableTestSourceFactory::Create();
-        }
+#else
+        case SourceType::ConfigurableTest:


florelis · 2026-06-29T23:59:03Z

+        // If the source type was empty, the constructor of sourceDetails will have defaulted it
+        AICLI_LOG(Repo, Info, << "Adding source: Name[" << sourceDetails.Name << "], Type[" << SourceTypeEnumToString(sourceDetails.Type) << "], Arg[" << sourceDetails.Arg << "]");


I wonder if it would be worth it to keep the semantics of "type can be empty and that means default" to simplify the changes in logic for this PR. I'm not sure how much we win changing the meaning now, and whether we may be missing some place.

That's actually why I added the comment - because I wanted it to be clear that by movng to a strongly typed enum here we would be losing some of that nuance of "Type can be empty"

I'm sure that I could find a way to keep that nuance, but semantically I'd rather always have an explicit type, that way there isn't any potential confusion about what empty actually means. If empty is passed into the constructor it gets defaulted, and IMO that makes all the downstream logic simpler because callsites don't have to worry whether it is empty or not, it becomes a case they don't need to worry about.

florelis · 2026-06-30T00:02:31Z

-            if (sourceType.empty()
-                || Utility::CaseInsensitiveEquals( Repository::Microsoft::PredefinedInstalledSourceFactory::Type(), sourceType))
+            auto sourceTypeEnum = ::AppInstaller::Repository::TryConvertToSourceTypeEnum(sourceType).value_or(::AppInstaller::Repository::Source::GetDefaultSourceType());
+            if (sourceTypeEnum == ::AppInstaller::Repository::SourceType::PreIndexedPackage || sourceTypeEnum == ::AppInstaller::Repository::SourceType::PredefinedInstalled)


I'm not sure what this does, but the logic changed. If you get the 'preindexed package' as input, you will execute this conditional.

This is an artifact of not allowing an empty source type. If sourceType were empty in this new flow, TryConvertToSourceTypeEnum is an empty optional and, according to the old logic, means 'default'. This is handled by the .value_or setting it to the default.

Then, because the source type could be default or predefined installed, I check both.

In either case, I believe that the PreIndexed package is intended to follow into this conditional as it is setting the weights for the progress bar which depend on downloading and installing the index, which are both part of the pre-indexed package flow. Excluding it here would cause it to not have the same behavior

Copilot

Pull request overview

Refactors internal source-type handling across Repository, CLI, and COM layers to use a strongly-typed AppInstaller::Repository::SourceType enum, keeping string conversion only at external boundaries (CLI args, JSON, Group Policy, COM).

Changes:

Introduces SourceType enum plus centralized string↔enum conversion helpers, and updates SourceDetails.Type and source factory routing to use the enum.
Updates source list serialization/parsing and Group Policy handling to convert at boundaries while keeping internal comparisons type-safe.
Updates unit tests and fixtures to use canonical source type values and adds CLI validation for source add --type.

Reviewed changes

Copilot reviewed 45 out of 45 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/Microsoft.Management.Deployment/PackageManager.cpp	COM Add/Remove catalog paths updated to enum and string conversion boundaries
src/Microsoft.Management.Deployment/PackageCatalogReference.cpp	Progress sink creation now stringifies enum type
src/Microsoft.Management.Deployment/PackageCatalogProgress.cpp	Progress sink selection now normalizes type via enum conversion
src/Microsoft.Management.Deployment/PackageCatalogInfo.cpp	COM-facing catalog type now returns canonical string from enum
src/AppInstallerRepositoryCore/SourcePolicy.h	Group Policy APIs updated to take `SourceType`
src/AppInstallerRepositoryCore/SourcePolicy.cpp	Policy matching/logging updated for enum-based source types
src/AppInstallerRepositoryCore/SourceList.h	Well-known source matching updated to accept `SourceType`
src/AppInstallerRepositoryCore/SourceList.cpp	Source YAML serialization/parsing and well-known checks migrated to enum
src/AppInstallerRepositoryCore/SourceFactory.h	`ISourceFactory::GetForType` now takes `SourceType`
src/AppInstallerRepositoryCore/Rest/RestSourceFactory.cpp	Rest source factory now enforces enum type equality
src/AppInstallerRepositoryCore/RepositorySource.cpp	Core enum conversion helpers + enum-based factory routing and constructors
src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h	Adds `SourceType` enum and conversion function declarations; `SourceDetails.Type` becomes enum
src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp	Package tracking source creation/removal updated to enum routing
src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp	PreIndexed factory now enforces `SourceType::PreIndexedPackage`
src/AppInstallerRepositoryCore/Microsoft/PredefinedWriteableSourceFactory.cpp	PredefinedWriteable factory now enforces enum type
src/AppInstallerRepositoryCore/Microsoft/PredefinedInstalledSourceFactory.cpp	PredefinedInstalled factory now enforces enum type
src/AppInstallerCLITests/TestSource.h	Test source details updated to enum type
src/AppInstallerCLITests/TestSource.cpp	Test helper source creation updated for enum constructor
src/AppInstallerCLITests/TestHooks.h	Test hook factory override now keyed by `SourceType`
src/AppInstallerCLITests/TestData/ImportFile-Good.json	Test fixture updated to canonical type string
src/AppInstallerCLITests/TestData/ImportFile-Good-WithOverrideArgs.json	Test fixture updated to canonical type string
src/AppInstallerCLITests/TestData/ImportFile-Good-WithLicenseAgreement.json	Test fixture updated to canonical type string
src/AppInstallerCLITests/TestData/ImportFile-Good-WithCustomSwitches.json	Test fixture updated to canonical type string
src/AppInstallerCLITests/TestData/ImportFile-Good-MachineScope.json	Test fixture updated to canonical type string
src/AppInstallerCLITests/TestData/ImportFile-Good-Dependencies.json	Test fixture updated to canonical type string
src/AppInstallerCLITests/TestData/ImportFile-Good-AlreadyInstalled.json	Test fixture updated to canonical type string
src/AppInstallerCLITests/TestData/ImportFile-Bad-UnknownSource.json	Test fixture updated to canonical type string
src/AppInstallerCLITests/TestData/ImportFile-Bad-UnknownPackageVersion.json	Test fixture updated to canonical type string
src/AppInstallerCLITests/TestData/ImportFile-Bad-UnknownPackage.json	Test fixture updated to canonical type string
src/AppInstallerCLITests/SQLiteIndexSource.cpp	SQLite test source details updated to enum type
src/AppInstallerCLITests/Sources.cpp	Source-list tests updated + new enum conversion test coverage
src/AppInstallerCLITests/SourceFlow.cpp	Workflow tests updated and adds CLI source-type validation tests
src/AppInstallerCLITests/PreIndexedPackageSource.cpp	PreIndexed tests updated to enum type
src/AppInstallerCLITests/PredefinedInstalledSource.cpp	PredefinedInstalled tests updated to enum type
src/AppInstallerCLITests/PackageTrackingCatalog.cpp	Tracking catalog tests updated to enum type
src/AppInstallerCLITests/PackageCollection.cpp	PackageCollection tests updated for enum/string boundary behavior
src/AppInstallerCLITests/CompositeSource.cpp	Tracking factory test hook updated to enum keying
src/AppInstallerCLITests/ARPChanges.cpp	Test hook override updated to enum keying
src/AppInstallerCLICore/Workflows/SourceFlow.cpp	CLI workflows updated to normalize type to enum and stringify for display/policy
src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp	Configuration units now use canonical type string via enum conversion
src/AppInstallerCLICore/PackageCollection.cpp	PackageCollection JSON read/write updated to convert type at boundary
src/AppInstallerCLICore/ConfigurationWingetDscModuleUnitValidation.cpp	DSC validation compares canonical enum string against input strings
src/AppInstallerCLICore/Commands/SourceCommand.cpp	Adds stricter `source add --type` validation using enum conversion
src/AppInstallerCLICore/Commands/DscSourceResource.cpp	DSC resource output stringifies enum type
doc/ReleaseNotes.md	Notes refactor and stronger validation in release notes

            std::string name = winrt::to_string(options.Name());
-            std::string type = winrt::to_string(options.Type());
+            auto sourceType = ::AppInstaller::Repository::TryConvertToSourceTypeEnum(winrt::to_string(options.Type())).value_or(::AppInstaller::Repository::Source::GetDefaultSourceType());
            std::string sourceUri = winrt::to_string(options.SourceUri());


+            auto sourceType = ::AppInstaller::Repository::TryConvertToSourceTypeEnum(winrt::to_string(options.Type())).value_or(::AppInstaller::Repository::Source::GetDefaultSourceType());
+            auto packageCatalogProgressSink = winrt::Microsoft::Management::Deployment::ProgressSinkFactory::CreatePackageCatalogProgressSink(
+                std::string{ ::AppInstaller::Repository::SourceTypeEnumToString(sourceType) },
+                report_progress);


Trenly and others added 4 commits June 21, 2026 16:08

Revert unintentional whitespace change

c9ff483

Spaces instead of tabs

9b9695b

Remove noops

31af78c

Trenly marked this pull request as ready for review June 21, 2026 21:30

Trenly requested a review from a team as a code owner June 21, 2026 21:30

Merge branch 'master' into trenly/source-type-enum-4463

e79d3e3

florelis reviewed Jun 30, 2026

View reviewed changes

florelis requested a review from Copilot July 14, 2026 21:54

Copilot started reviewing on behalf of florelis July 14, 2026 21:55 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes


		## Bug Fixes

		* Refactored source type handling to use a `SourceType` enum with centralized string conversion, and added stronger `source add` type validation (#4463).

		// Defaults to PreIndexedPackage if not specified, moved here from call sites during migration from
		// String type to Enum type to avoid conversion in multiple places.

		// If the source type was empty, the constructor of sourceDetails will have defaulted it
		AICLI_LOG(Repo, Info, << "Adding source: Name[" << sourceDetails.Name << "], Type[" << SourceTypeEnumToString(sourceDetails.Type) << "], Arg[" << sourceDetails.Arg << "]");

Uh oh!

Conversation

Trenly commented Jun 21, 2026 • edited by microsoft-github-policy-service Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📖 Description

🔗 References

🔍 Validation

✅ Checklist

📋 Issue Type

Microsoft Reviewers: Open in CodeFlow

Uh oh!

florelis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trenly Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Trenly commented Jun 21, 2026 •

edited by microsoft-github-policy-service Bot

Loading

Trenly Jun 30, 2026 •

edited

Loading